Skip to content

Add Unit Tests and Improve Documentation for utils.py clip_tokens Function#3

Open
TaskerJang wants to merge 2 commits intomainfrom
feature/clip-tokens-tests-and-docs
Open

Add Unit Tests and Improve Documentation for utils.py clip_tokens Function#3
TaskerJang wants to merge 2 commits intomainfrom
feature/clip-tokens-tests-and-docs

Conversation

@TaskerJang
Copy link

@TaskerJang TaskerJang commented May 22, 2025

Pull Request

💡 PR 요약

Issue #1793에서 요청된 clip_tokens 함수의 단위 테스트 추가 및 문서화 개선 작업을 완료했습니다. 기존 함수의 안정성을 해치지 않으면서 테스트 커버리지를 대폭 확장하고 개발자 경험을 향상시켰습니다.

🔍 주요 변경사항

  • tests/unittest/test_clip_tokens.py에 21개의 포괄적인 단위 테스트 추가
  • pr_agent/algo/utils.pyclip_tokens 함수 docstring 완전 개선
  • 엣지 케이스, 매개변수 조합, 예외 처리 등 모든 시나리오 테스트 커버
  • 4개의 실용적인 사용 예제와 상세한 매개변수 설명 추가
  • 기존 함수 로직은 변경하지 않아 하위 호환성 완벽 유지

🔗 연관된 이슈

qodo-ai#1793

📸 스크린샷 (선택)

테스트코드

✅ 체크리스트

  • 테스트 코드를 작성하였나요? (21개 포괄적 테스트)
  • 관련 문서를 업데이트하였나요? (완전한 docstring)
  • Breaking Change가 있나요? (기존 기능 완벽 호환)
  • 코드 포맷팅과 린트 검사를 완료하였나요?

🙏 리뷰어 참고사항

  • 기존 함수 로직은 전혀 수정하지 않고 문서화만 개선하여 안전성 보장
  • 모든 테스트가 통과하여 기존 기능과의 호환성 확인
  • Issue #1793의 세 가지 주요 요구사항 모두 충족

📋 추가 컨텍스트 (선택)

이번 작업으로 clip_tokens 함수의 신뢰성이 크게 향상되었습니다. 21개의 테스트 케이스는 유니코드 처리, 긴 텍스트 처리, 다양한 매개변수 조합 등을 모두 검증하며, 향후 함수 수정 시 회귀 테스트 역할을 할 수 있습니다. 또한 완전히 개선된 docstring으로 다른 개발자들이 함수를 올바르게 사용할 수 있도록 충분한 가이드를 제공합니다.

TaskerJang added 2 commits May 22, 2025 14:33
- 21개 테스트 케이스로 엣지 케이스 및 매개변수 조합 검증
- 유니코드, 특수문자, 긴 텍스트 처리 테스트 포함
- 예외 상황 및 하위 호환성 확인
- 모든 매개변수에 대한 완전한 설명 추가
- 4개의 실용적인 사용 예제 포함
- 반환값 조건 및 동작 명시
- 안전 계수(0.9) 및 오류 처리 설명
@KangmoonSeo
Copy link
Member

수고 많으셨습니다!! 메인테이너가 엄청나게 좋아할 것 같아요. ㅋㅋㅋㅋㅋㅎㅎ
PR 올리시기 전에 놓치실 수 있는 부분을 짚자면..! 한글로 된 커밋 메시지를 실 PR에서는 영어로 올려주시면 최고일 것 같아요. 😀


# Generated by CodiumAI

import pytest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Takser..God 님, 코드 보면서 개인적으로 궁금한 점이 생겼어요 :)
test_clip_tokens.py처럼 꼼꼼한 테스트 케이스들을 보면
구상하실 때 어떤 것부터 시작해서 살을 붙여나는지 그 과정이 궁금합니다.

함수 핵심 기능을 먼저 잡고,
그다음에 입력 값 떠올고,
옵션별 동작이나 예외 처리 순으로 체계적으로 만들어가시는 건지
아니면 개인이 주로 사용하는 방식이 있는지 등등...
빌드업 과정에 대한 생각의 흐름 팁을 조금만 공유해 주실 수 있을까요?
(아무래도 제가 정량적인 경험이 적다보니... 생각의 흐름도가 궁금합니다!)

항상 많이 배웁니다🙏🔥🔥🔥

Copy link
Author

@TaskerJang TaskerJang May 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. 일단 함수 보고 "뭐하는 함수지?"

def clip_tokens(text: str, max_tokens: int, add_three_dots=True...)

토큰 자르는 함수구나. 그럼 일단 기본적인 거부터 테스트해보자.

2. 기존 테스트 파일 봤더니 2개밖에 없음

def test_clip(self):
    text = "line1\nline2..."
    # 이것만 있네?

이거 너무 부족하다 싶어서 Issue 내용 다시 봤어요.

3. Issue에서 요구한 것들 체크

  • "edge cases" → 아 빈 문자열, 음수 같은 거 테스트해야겠다
  • "parameter combinations" → add_three_dots, delete_last_line 조합들
  • "error handling" → 예외 상황들

4. 실제로는 하나씩 만들면서 "아 이것도 테스트해야겠네" 하면서 추가

def test_empty_input_text(self):  # 일단 빈 문자열
def test_negative_max_tokens(self):  # 음수 넣으면 어떻게 될까?

이렇게 하나씩 만들다가 "아 유니코드는?" "0으로 나누기는?" 이런 식으로 계속 떠올라서 추가한 거예요.

5. 솔직히 21개나 될 줄 몰랐음 ㅋㅋ

처음엔 10개 정도 생각했는데 하다보니 "이것도 테스트해야겠네" 하면서 계속 늘어났어요.

특히 mock 테스트 부분은... 사실 좀 헤맸어요. TokenEncoder 어떻게 mock 해야 하나 고민하면서 여러 번 시도해봤고요.

진짜 완벽하게 계획 세우고 한 게 아니라 만들면서 "아 이것도, 저것도" 하면서 덧붙인 거라 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고스트바둑왕처럼
1->2->3->4->5 이렇게 빙의하면서 따라가니까 이해가 갔습니다..
Issue에서 요구한 것들 체크 이 부분도 중요하고
결국 하나 하나 붙여나가는게 방법이었군요!
(킬링 포인트ㅋㅋ) - > 솔직히 21개나 될 줄 몰랐음

텍스트 안잘리고 제대로 나오는지, 텍스트 잘리고+ 붙는지, 0으로 나눠지는 오류 테스트 등...
바로 나온게 아니라 한땀 한땀, 여러 번 고민이랑 시도가 들어가서 나온거군요
덕분에 많이 배워갑니다! ✍️

(활동 이후에도 블로그 글 몰래 확인하겠습니다..)

result = clip_tokens(text, max_tokens)
expected_results = 'line1\nline2\nline3\n\n...(truncated)'
assert result == expected_results
assert result == expected_results No newline at end of file
Copy link

@Kkan9ma Kkan9ma May 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

소소하지만 EOF 도 챙겨주십사.. 코멘트 남겨봅니다 :)

HOPARKSUNG added a commit that referenced this pull request May 26, 2025
HOPARKSUNG pushed a commit that referenced this pull request May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants